-
Notifications
You must be signed in to change notification settings - Fork 13k
fix: add multiple file support to /sendEmailAttachment #38183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add multiple file support to /sendEmailAttachment #38183
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: b41f2cc The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughRefactors outgoing email handling to support multiple file attachments by batching upload retrieval and building Mail.Attachments; adds Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Command
participant EmailBox as EmailInbox Service
participant Uploads as Uploads Model
participant FileUpload as FileUpload Service
participant Mail as Mail Service
User->>EmailBox: invoke sendEmailAttachment(message, room)
EmailBox->>EmailBox: extract fileRefs from message.files / message.file
EmailBox->>Uploads: findByIds(fileRefs)
Uploads-->>EmailBox: returns IUpload[]
loop for each upload
EmailBox->>FileUpload: getBuffer(upload)
FileUpload-->>EmailBox: returns buffer
EmailBox->>EmailBox: buildMailAttachment(upload, buffer)
end
EmailBox->>EmailBox: compose emailText from attachment descriptions
EmailBox->>Mail: send email with attachments + emailText (+ inReplyTo/references)
Mail-->>EmailBox: send result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38183 +/- ##
===========================================
- Coverage 70.73% 70.70% -0.03%
===========================================
Files 3147 3147
Lines 108874 108882 +8
Branches 19597 19574 -23
===========================================
- Hits 77007 76982 -25
- Misses 29863 29896 +33
Partials 2004 2004
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 4 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.ts`:
- Around line 150-165: sendEmail may resolve to undefined on failure, so the
chained .then((info) => LivechatRooms.updateEmailThreadByRoomId(room._id,
info.messageId)) can throw when info is undefined; update the call site around
sendEmail to handle failures explicitly — either await sendEmail inside a
try/catch and only call LivechatRooms.updateEmailThreadByRoomId on a successful
result, or chain a .catch to log/handle the error and guard the .then with a
null check (e.g. only call LivechatRooms.updateEmailThreadByRoomId when info is
truthy). Ensure you reference sendEmail and
LivechatRooms.updateEmailThreadByRoomId in the fix.
🧹 Nitpick comments (1)
apps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.ts (1)
137-142: Consider using the newfindByIdsmethod for consistency.The PR adds
findByIdsto the Uploads model, but this code uses a directfindwith$in. Using the new method would align with the model changes and improve readability.Suggested change
- const files = await Uploads.find({ _id: { $in: fileRefs.map((f) => f._id) } }).toArray(); + const files = await Uploads.findByIds(fileRefs.map((f) => f._id)).toArray();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/chatty-camels-explain.mdapps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.tspackages/model-typings/src/models/IBaseUploadsModel.tspackages/models/src/models/BaseUploadModel.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/models/src/models/BaseUploadModel.tspackages/model-typings/src/models/IBaseUploadsModel.tsapps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.ts
🧬 Code graph analysis (1)
apps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.ts (2)
packages/core-typings/src/IRoom.ts (1)
IOmnichannelRoom(269-320)packages/models/src/index.ts (2)
Uploads(202-202)LivechatRooms(167-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (6)
packages/models/src/models/BaseUploadModel.ts (1)
95-103: LGTM!The
findByIdsimplementation is clean and follows the established patterns in this model class. The$inquery is the standard MongoDB approach for batch retrieval by IDs.packages/model-typings/src/models/IBaseUploadsModel.ts (1)
13-14: LGTM!The interface addition is correctly typed and aligns with the implementation in
BaseUploadModel.ts.apps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.ts (3)
25-35: LGTM!The
buildMailAttachmenthelper is well-structured: it correctly handles the case where no buffer is available and returns a properly typedMail.Attachmentobject.
117-120: LGTM!Good backward compatibility handling—supports both the newer
message.filesarray and the legacymessage.filesingle-file format.
144-148: LGTM!Composing the email body from attachment descriptions is a reasonable approach for multi-attachment messages.
.changeset/chatty-camels-explain.md (1)
1-7: LGTM!The changeset correctly identifies all affected packages and accurately describes the fix.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Proposed changes (Including videos or screenshots)
As per CORE-1625, this PR adds support for sending multiple file attachments via the
/sendEmailAttachmentslash command.Previously, when a message contained multiple files, only the first file would be sent as an email attachment. With this change, all files attached to the message are included in the email.
Issues
Steps to test or reproduce
Prerequisites
1. Set up email testing environment
Create a
docker-compose.mail-test.ymlfile:Start the containers:
2. Configure email inbox in Rocket.Chat
/admin/email-inboxes)(GreenMail auto-creates accounts)
3. Test single file attachment (baseline)
test@localhostusing Roundcube (http://localhost:8080)visitor@test.com/visitor)test@localhostvisitor@test.com, and verify:4. Test multiple file attachments (new feature)
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.